Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collection docs #27902

Closed
wants to merge 3 commits into from
Closed

Collection docs #27902

wants to merge 3 commits into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 19, 2015

Two commits:

  1. Clarify what Vec does and doesn't precisely guarantee. People are already relying on this stuff, so we might as well call it a day and specify it. This commit deserves serious scutiny.

  2. Remove references to deprecated collections from the top level collections docs.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

This all looks good to me, and I'm on board with it, but documenting that we'll never to a small vector optimization probably wants some more visibility, so:

cc @rust-lang/libs

@alexcrichton alexcrichton added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 20, 2015
/// uninitialized elements.
///
/// Vec will never perform a "small optimization" where elements are actually
/// stored on the stack for two reasons:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the stack is kind of the wrong terminology. The vec is just a value, and it can live anywhere, so "inline" is more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@bors
Copy link
Contributor

bors commented Aug 26, 2015

☔ The latest upstream changes (presumably #27998) made this pull request unmergeable. Please resolve the merge conflicts.

/// overriding their defaults may change the behavior.
///
/// Most fundamentally, Vec is and always will be a (pointer, capacity, length)
/// triplet. No more, no less. The order of these fields is completely
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this also be true if we ever add a allocator type paramter? Possible designs I've seen for this might add an additional field to the type:

struct Vec<T, A: Allocator=Heap> {
    data: *mut T,
    len: usize,
    capacity: usize,
    allocator: A::HandleData,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right above this:

Note that these guarantees refer to an unqualified Vec<T>.
If additional type parameters are added (e.g. to support custom allocators),
overriding their defaults may change the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(allocator would be a ZST by default, so its only impact could be ordering, which is already unspecified)

@Kimundi
Copy link
Member

Kimundi commented Aug 26, 2015

As a additional data point against allowing smallvector optimizations: My owning-ref crate currently assumes that the backing storage of a Vec, Rc and Box is at a stable address in order to be memory safe.

@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the conclusion was to r+ with a rebase.

@alexcrichton alexcrichton removed the I-needs-decision Issue: In need of a decision. label Sep 17, 2015
@steveklabnik
Copy link
Member

@gankro are you going to rebase this, or do you want a hand? I know you've been really busy.

@Gankra
Copy link
Contributor Author

Gankra commented Sep 25, 2015

@steveklabnik please do!

@steveklabnik steveklabnik mentioned this pull request Oct 1, 2015
@steveklabnik
Copy link
Member

Superseded by #28797

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Oct 1, 2015
…lexcrichton

This is a rebase of rust-lang#27902, since @gankro  is busy ❤️
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 2, 2015
…lexcrichton

This is a rebase of rust-lang#27902, since @gankro  is busy ❤️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants